Feature: Git sync, set-tracking, and push divergence handling#796
Conversation
- Create push-service.ts replacing unsafe child_process.exec with safe execGitCommand array args; detect diverged branches and auto-resolve via pull-then-retry when autoResolve is enabled - Create sync-service.ts composing pull then push with automatic stash management and divergence retry - Add POST /set-tracking endpoint to set upstream tracking branch - Add POST /sync endpoint for pull+push in one operation - Refactor push route to delegate to push-service (thin handler pattern) - Add sync(), setTracking() to frontend API client, mutations, and hooks - Add Sync menu item (with split-button for multiple remotes) between Push and Merge & Rebase in the worktree actions dropdown - Add "Set as Tracking Branch" sub-menu on each remote in Pull/Push remote submenus (no duplicate remote lists) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Git integration by modernizing push operations, introducing a comprehensive sync feature, and enabling explicit tracking branch management. These changes aim to provide a more robust, safer, and user-friendly experience for managing Git repositories, particularly when dealing with divergent branches and synchronizing changes with remotes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds server endpoints and services for worktree sync and tracking, refactors push to a push-service with auto-resolve, and exposes sync/set-tracking actions through UI components, hooks, and API client/type updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Worktree Panel
participant Hook as useWorktreeActions
participant Mutation as useSyncWorktree
participant API as HTTP API Client
participant Server as /api/worktree/sync
participant SyncSvc as Sync Service
participant PullSvc as Pull Service
participant PushSvc as Push Service
participant Git as Git Worktree
User->>UI: Click "Sync"
UI->>Hook: handleSync(worktree, remote)
Hook->>Mutation: mutate({ worktreePath, remote })
Mutation->>API: POST /api/worktree/sync
API->>Server: HTTP request
Server->>SyncSvc: performSync(worktreePath, { remote })
SyncSvc->>PullSvc: performPull(..., stashIfNeeded)
PullSvc->>Git: git pull
Git-->>PullSvc: pull result (ok/conflict)
PullSvc-->>SyncSvc: pull result
SyncSvc->>PushSvc: performPush(..., autoResolve?)
PushSvc->>Git: git push
Git-->>PushSvc: push result (ok/diverged/error)
PushSvc-->>SyncSvc: push result
SyncSvc-->>Server: SyncResult
Server-->>API: JSON response
API-->>Mutation: success/error
Mutation->>UI: invalidate queries, show toast
UI->>User: Display result
sequenceDiagram
actor User
participant UI as Worktree Panel
participant Hook as useWorktreeActions
participant Mutation as usePushMutation
participant API as HTTP API Client
participant Server as /api/worktree/push
participant PushSvc as Push Service
participant Git as Git Worktree
User->>UI: Click "Push" (autoResolve)
UI->>Hook: handlePush(worktree, remote, autoResolve)
Hook->>Mutation: mutate({ worktreePath, remote, autoResolve })
Mutation->>API: POST /api/worktree/push {autoResolve:true}
API->>Server: HTTP request
Server->>PushSvc: performPush(worktreePath, { remote, autoResolve })
PushSvc->>Git: getCurrentBranch()
Git-->>PushSvc: branch name / detached
PushSvc->>Git: git push
Git-->>PushSvc: divergence error
alt autoResolve == true
PushSvc->>PushSvc: performPull({ stashIfNeeded })
PushSvc->>Git: git pull
Git-->>PushSvc: pull result
PushSvc->>Git: git push (retry)
Git-->>PushSvc: push result
end
PushSvc-->>Server: PushResult
Server-->>API: JSON response
API-->>Mutation: success/error
Mutation->>UI: invalidate queries, show toast
UI->>User: Show push outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the Git workflow by adding sync and set-tracking functionalities, and modernizing the push operation. The changes are well-structured, with business logic correctly encapsulated in new services on the backend. The frontend updates are comprehensive, wiring up the new capabilities through the component hierarchy. Overall, this is a solid feature addition. I've identified a couple of areas for improvement: one regarding redundant logic in the new push-service and another concerning code duplication in a React component, which could be refactored for better maintainability.
| // 6b. Non-divergence error - try with --set-upstream | ||
| try { | ||
| const setUpstreamArgs = ['push', '--set-upstream', targetRemote, branchName]; | ||
| if (force) { | ||
| setUpstreamArgs.push('--force'); | ||
| } | ||
| await execGitCommand(setUpstreamArgs, worktreePath); | ||
|
|
||
| return { | ||
| success: true, | ||
| branch: branchName, | ||
| pushed: true, | ||
| message: `Successfully pushed ${branchName} to ${targetRemote} (set upstream)`, | ||
| }; | ||
| } catch (upstreamError: unknown) { | ||
| const upstreamErr = upstreamError as { stderr?: string; message?: string }; | ||
| return { | ||
| success: false, | ||
| branch: branchName, | ||
| pushed: false, | ||
| error: upstreamErr.stderr || upstreamErr.message || getErrorMessage(pushError), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The fallback logic to try git push --set-upstream is redundant. The initial push attempt on line 113 already uses the -u flag, which is an alias for --set-upstream. If the first push attempt fails for a reason other than divergence, this second attempt will also fail for the same reason.
To improve clarity and remove redundant code, it would be better to remove this try/catch block and just return the original error if it's not a divergence error.
// 6b. Non-divergence error
return {
success: false,
branch: branchName,
pushed: false,
error: err.stderr || err.message || getErrorMessage(pushError),
};| {remotes.map((remote) => | ||
| onSetTracking ? ( | ||
| <DropdownMenuSub key={remote.name}> | ||
| <div className="flex items-center"> | ||
| <DropdownMenuItem | ||
| onClick={() => | ||
| isGitOpsAvailable && onPushWithRemote(worktree, remote.name) | ||
| } | ||
| disabled={isPushing || !isGitOpsAvailable} | ||
| className="text-xs flex-1 pr-0 rounded-r-none" | ||
| > | ||
| <Upload className="w-3.5 h-3.5 mr-2" /> | ||
| {remote.name} | ||
| {trackingRemote === remote.name && ( | ||
| <span className="ml-auto text-[10px] text-muted-foreground mr-1"> | ||
| tracking | ||
| </span> | ||
| )} | ||
| </DropdownMenuItem> | ||
| <DropdownMenuSubTrigger | ||
| className="text-xs px-1 rounded-l-none border-l border-border/30 h-8" | ||
| disabled={!isGitOpsAvailable} | ||
| /> | ||
| </div> | ||
| <DropdownMenuSubContent> | ||
| <DropdownMenuItem | ||
| onClick={() => isGitOpsAvailable && onSetTracking(worktree, remote.name)} | ||
| disabled={!isGitOpsAvailable} | ||
| className="text-xs" | ||
| > | ||
| <GitBranch className="w-3.5 h-3.5 mr-2" /> | ||
| Set as Tracking Branch | ||
| </DropdownMenuItem> | ||
| </DropdownMenuSubContent> | ||
| </DropdownMenuSub> | ||
| ) : ( | ||
| <DropdownMenuItem | ||
| key={remote.name} | ||
| onClick={() => isGitOpsAvailable && onPushWithRemote(worktree, remote.name)} | ||
| disabled={isPushing || !isGitOpsAvailable} | ||
| className="text-xs" | ||
| > | ||
| <Upload className="w-3.5 h-3.5 mr-2" /> | ||
| {remote.name} | ||
| <span className="ml-auto text-[10px] text-muted-foreground max-w-[100px] truncate"> | ||
| {remote.url} | ||
| </span> | ||
| </DropdownMenuItem> | ||
| ) | ||
| )} |
There was a problem hiding this comment.
This block of code for rendering the push remote items is nearly identical to the block for rendering the pull remote items (lines 734-783). This duplication makes the component harder to maintain.
To improve maintainability, consider extracting this logic into a reusable component, for example RemoteActionMenuItem. This component could accept props like the remote, the action handler (onPullWithRemote or onPushWithRemote), the loading state (isPulling or isPushing), and the icon (Download or Upload) to render the appropriate menu item.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
apps/ui/src/types/electron.d.ts (1)
1001-1020: Consider whetherdiverged?: booleanshould be surfaced at the top level ofsync().The
push()return type exposes a top-leveldiverged?: booleanfor cases where auto-resolve fails.sync()omits this field. In the normal flow the pull phase resolves divergence, but a narrow race (remote advances again between pull and push) could leave consumers unable to distinguish a divergence error from any other error without parsing the error string.If the server guarantees that scenario cannot occur (e.g. push-service retries are bounded and any remaining divergence becomes a generic error), this omission is intentional — a brief comment in the type or JSDoc would confirm that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/types/electron.d.ts` around lines 1001 - 1020, Add a top-level optional diverged?: boolean to the sync() return shape (the Promise result object) to mirror push() and allow callers to detect a push-divergence race without parsing error strings; update the sync declaration (function name: sync) to include diverged?: boolean alongside the existing success/error/hasConflicts fields, or alternatively add a JSDoc note on sync explaining why divergence is impossible if you intend to keep the current type.apps/ui/src/lib/electron.ts (1)
2303-2319: Hardcoded branch fallback in mock can mislead during development.When
branchisundefined, the real server resolves the current branch dynamically. The mock always returns'feature-branch', which may confuse developers testing the "no branch specified" path. A small comment or a more descriptive placeholder (e.g.,'current-branch') would clarify intent.✏️ Suggested tweak
- const targetBranch = branch || 'feature-branch'; + // In production the server resolves to the current branch; mock uses a placeholder + const targetBranch = branch || 'current-branch';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/electron.ts` around lines 2303 - 2319, The mock implementation of setTracking uses a misleading hardcoded fallback 'feature-branch' when branch is undefined; update the function (setTracking) to use a clearer placeholder like 'current-branch' or add a comment indicating this is a dynamic stand-in for the real server-resolved branch, and ensure the returned result fields (branch, upstream, message) reflect that placeholder so developers testing the "no branch specified" path aren't confused by a concrete feature name.apps/ui/src/hooks/mutations/use-worktree-mutations.ts (1)
208-227:useSyncWorktreediscards structured conflict information on failure.The API returns
hasConflicts,conflictFiles, andconflictSource('pull' | 'stash') at the top level, but the mutation only forwards theerrorstring. Sync spans two phases (pull + stash-pop + push), so the source of a conflict is more actionable here than for a simple pull or push.While the existing
usePullWorktreeandusePushWorktreehave the same gap, sync is a higher-level operation where callers would benefit most from this context — for example, to show "conflict occurred while restoring stash" vs. "conflict occurred while pulling".♻️ Proposed improvement
- if (!result.success) { - throw new Error(result.error || 'Failed to sync'); - } - return result.result; + if (!result.success) { + const err = Object.assign( + new Error(result.error || 'Failed to sync'), + { + hasConflicts: result.hasConflicts, + conflictFiles: result.conflictFiles, + conflictSource: result.conflictSource, + } + ); + throw err; + } + return result.result;Then in
onError, the enriched fields can be inspected similarly to howuseSwitchBranchsurfacesstashPopConflicts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/mutations/use-worktree-mutations.ts` around lines 208 - 227, The mutation currently throws a plain Error with only result.error, discarding structured fields like hasConflicts, conflictFiles, and conflictSource returned by the API; update the mutationFn in use-worktree-mutations (the useSyncWorktree mutation) to throw the original result object (or a custom error object that preserves result.hasConflicts, result.conflictFiles, result.conflictSource and result.error) instead of new Error(result.error), and adjust the onError handler signature in the same file to accept the thrown object (inspect error.hasConflicts, error.conflictFiles, error.conflictSource) so callers can differentiate conflictSource (e.g., 'pull' vs 'stash') when handling sync failures.apps/server/src/routes/worktree/routes/sync.ts (1)
35-46: Consider includingbranchin error response for client context.
performSyncpopulatesresult.branchin most failure paths (pull failure, conflict, push-diverge retry). Omitting it from the error response means the client receives no branch context on failure. A simple addition:💡 Suggested improvement
res.status(statusCode).json({ success: false, + branch: result.branch, error: result.error, hasConflicts: result.hasConflicts, conflictFiles: result.conflictFiles, conflictSource: result.conflictSource, pulled: result.pulled, pushed: result.pushed, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/sync.ts` around lines 35 - 46, The error response JSON in the sync route currently omits branch context even though performSync sets result.branch on failures; update the error response in the failure block (the code that checks if (!result.success) and builds the JSON with success, error, hasConflicts, etc.) to include branch: result.branch so the client receives branch information on pull/conflict/push-diverge failures.apps/server/src/routes/worktree/routes/set-tracking.ts (1)
15-75: Business logic in route handler violates server layering guideline.
getCurrentBranch()andexecGitCommand()are called directly in the route handler. The analogous/syncroute correctly delegates all Git logic tosync-service.ts. Aset-tracking-service.ts(or addingperformSetTracking()to an existing service) would keep the server layer consistent and testable.As per coding guidelines: "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/set-tracking.ts` around lines 15 - 75, Move all Git/business logic out of createSetTrackingHandler and into a new service (e.g., services/set-tracking-service.ts) or add a performSetTracking(worktreePath, remote, branch?) function to an existing service; the handler should only validate req.body, call the service function (which will call getCurrentBranch and execGitCommand, handle the detached HEAD check, and throw meaningful errors), and translate the service result to the HTTP response. Ensure the service returns a structured result matching the current response shape (branch, remote, upstream, message) and that the route catches service errors, logs with logError('Set tracking branch failed', error) and returns getErrorMessage(error) in the JSON error payload.apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts (1)
241-261:setTrackingMutation.isPendingis not exposed — UI cannot show loading state.
isSyncingis surfaced but there is noisSettingTracking, so consumers have no way to disable the "Set as Tracking Branch" menu item or show a spinner while the mutation is in-flight. The mutation itself prevents double-submission, but the missing state creates a silent UX gap.💡 Suggested addition
return { isPulling: pullMutation.isPending, isPushing: pushMutation.isPending, isSyncing: syncMutation.isPending, + isSettingTracking: setTrackingMutation.isPending, isSwitching: switchBranchMutation.isPending, ... };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts` around lines 241 - 261, The return object from use-worktree-actions.ts is missing the pending state for the setTrackingMutation; add an isSettingTracking property (set to setTrackingMutation.isPending) alongside the other flags so consumers can disable the "Set as Tracking Branch" UI and show a spinner while handleSetTracking runs; update the exported return to include isSettingTracking and ensure handleSetTracking remains unchanged.apps/server/src/routes/worktree/routes/push.ts (1)
34-44: HTTP 400 for a diverged push is inconsistent with the 409 used by/syncfor conflicts.
isClientErrormaps'diverged'(and'rejected') to HTTP 400. A diverged branch is not a malformed request — the client sent a validworktreePath/remotepair. The/syncroute correctly returns 409 Conflict when merge conflicts occur. Using 409 for diverged pushes too would align the two endpoints and give clients a uniform signal to retry after rebasing/pulling.💡 Suggested fix
- if (!result.success) { - const statusCode = isClientError(result.error ?? '') ? 400 : 500; - res.status(statusCode).json({ + if (!result.success) { + const statusCode = result.diverged || result.hasConflicts ? 409 : 500; + res.status(statusCode).json({ success: false, error: result.error, diverged: result.diverged, hasConflicts: result.hasConflicts, conflictFiles: result.conflictFiles, }); return; }And remove the now-unused
isClientErrorhelper.Also applies to: 67-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/push.ts` around lines 34 - 44, When handling a failed push in the push route (the block checking result.success and using isClientError), treat a result.error value of 'diverged' as HTTP 409 Conflict instead of 400: compute statusCode as 409 when result.error === 'diverged' (keep 400 for other client-side validation errors and 500 for server errors), update both error-response sites (the blocks around result.success and the later block at ~67-72) to use this mapping, and then remove the now-unused isClientError helper to avoid dead code.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
734-783: Nearly identical remote-submenu rendering in Pull and Push sections.The two blocks for rendering per-remote entries with "Set as Tracking Branch" are structurally identical — differing only in the icon (
Download/Upload), the handler (onPullWithRemote/onPushWithRemote), and the disabled-state source (isPulling/isPushing). If this pattern grows further (e.g., sync remotes also gaining set-tracking), consider extracting a sharedRemoteSubMenuItemscomponent to reduce duplication.Also applies to: 869-918
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 734 - 783, Extract the duplicated per-remote UI into a reusable component (e.g., RemoteSubMenuItems) and replace the two near-identical map blocks with calls to that component; the component should accept props: remotes, worktree, onAction (handler for clicking remote e.g., onPullWithRemote or onPushWithRemote), actionDisabled (boolean or function per-remote using isPulling/isPushing), actionIcon (Download/Upload), onSetTracking, trackingRemote, and isGitOpsAvailable, and render the same DropdownMenuItem/DropdownMenuSub structure (including the Set as Tracking Branch item) so the only differences between the Pull and Push usages are the icon prop, the onAction prop, and the actionDisabled prop.apps/server/src/services/sync-service.ts (2)
140-185: Double-pull scenario on divergence is acceptable but worth documenting.When push fails with divergence (Line 142), the retry calls
performPushwithautoResolve: true, which internally does another pull-then-push. This means the sync performs: pull → push (rejected) → pull (again) → push (retry). This is a sound one-retry strategy for handling a race between pull and push, but it's worth a brief inline comment noting the intentional double-pull so future maintainers don't try to "optimize" it away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/sync-service.ts` around lines 140 - 185, The retry branch that triggers when pushResult.diverged intentionally calls performPush with autoResolve: true which internally performs another pull-then-push, resulting in a pull → push (rejected) → pull → push sequence; update the code around the if (pushResult.diverged) block (reference pushResult.diverged and performPush) with a concise inline comment explaining that the second pull is intentional to resolve races and must not be removed or "optimized" away, and mention getErrorMessage/pullResult as context for the surrounding error handling.
68-208: Events for sync progress tracking are not currently emitted.The coding guidelines state that server operations should emit events via
createEventEmitter()for WebSocket streaming to the frontend. However,sync-service.ts(along withpull-service.tsandpush-service.ts) does not follow this pattern. Adding event emission would require passing anEventEmitterto the route handler and emitting progress events such assync:started,sync:pull-complete, andsync:completedto provide real-time UI feedback during the multi-step sync operation.Note: This is consistent with the existing pull/push family of operations, which are currently designed as simple request-response endpoints without event streaming. Implementing this would require aligning with operations like rebase and stash that already use this pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/sync-service.ts` around lines 68 - 208, performSync currently runs a multi-step pull+push flow without emitting progress events; update performSync to accept an EventEmitter (or create one via createEventEmitter passed from the route) and emit structured events at key points: emit 'sync:started' at start (include worktreePath and remote), 'sync:pull-complete' after performPull returns (include pullResult and any conflict info), 'sync:push-start' before performPush, 'sync:push-retry' when retrying with autoResolve, and terminal events 'sync:completed' or 'sync:failed' with the final SyncResult; ensure performPull and performPush call sites in this function remain unchanged but wire the emitter into retries and catch blocks so errors use getErrorMessage and the emitter to stream failures (reference performSync, performPull, performPush, getErrorMessage, and createEventEmitter).apps/server/src/services/push-service.ts (1)
112-116: Every push unconditionally sets-u(upstream tracking).Using
-uon every push means that pushing to any remote will silently re-configure the upstream tracking branch. If a user pushes to a secondary remote (e.g., a fork), this would change their tracking away from the primary remote. Given that this PR also adds explicitset-trackingfunctionality, the implicit-uhere could conflict with the user's intent.Consider reserving
-ufor the fallback path only (when no upstream is configured) or making it opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/push-service.ts` around lines 112 - 116, The pushArgs currently always include '-u', which can overwrite a branch's upstream; change the push preparation in push-service.ts so '-u' is only added when needed — either when the caller explicitly requests tracking (e.g., a new opt-in flag like setTracking) or when the branch has no configured upstream; to implement, before pushing check the branch's upstream status (via the existing helper or a git command) and only push with '-u', e.g., pushArgs = ['push', targetRemote, branchName] and conditionally pushArgs.unshift('-u') or pushArgs.push('-u') when upstream is absent or setTracking===true; keep the existing force handling on the pushArgs array.apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
834-856: Trivial pass-through wrappers can be simplified.
handleSyncWithRemoteSelection,handleSyncWithSpecificRemote, andhandleSetTrackingForRemoteare identity wrappers with no added logic. Unlike the analogoushandlePullWithRemoteSelection(which checks the tracking remote and may show a dialog),handleSyncWithRemoteSelectionjust forwards tohandleSync— so the name is misleading. You could passhandleSyncandhandleSetTrackingdirectly as props, or rename the sync wrapper to reflect that it doesn't perform remote selection.♻️ Simplification (pass directly)
Where these are used as props, replace them with the hook-provided handlers:
- onSync={handleSyncWithRemoteSelection} - onSyncWithRemote={handleSyncWithSpecificRemote} - onSetTracking={handleSetTrackingForRemote} + onSync={handleSync} + onSyncWithRemote={handleSync} + onSetTracking={handleSetTracking}And remove the three wrapper
useCallbacks at lines 834–856.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx` around lines 834 - 856, The three useCallback pass-throughs (handleSyncWithRemoteSelection, handleSyncWithSpecificRemote, handleSetTrackingForRemote) are unnecessary wrappers around handleSync and handleSetTracking; remove these three functions and update call sites to pass handleSync and handleSetTracking directly (or, if a different signature is required, rename handleSyncWithRemoteSelection to clearly indicate it doesn't perform selection). Ensure any props or handlers that previously expected the wrapper signatures now accept the original handleSync(worktree[, remote]) and handleSetTracking(worktree, remote) functions so there are no mismatched parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/set-tracking.ts`:
- Around line 34-60: The code uses
execGitCommand(['branch','--set-upstream-to',`${remote}/${targetBranch}`],
worktreePath) which only changes the upstream of the currently checked-out
branch; to apply the upstream to the named local branch you must pass that
branch name as the final positional argument. Update the execGitCommand
invocation in set-tracking.ts (the call using targetBranch) to include
targetBranch as the last argument (i.e. ['branch','--set-upstream-to',
`${remote}/${targetBranch}`, targetBranch]) and ensure targetBranch is
validated/derived the same way as before.
In `@apps/server/src/services/push-service.ts`:
- Around line 218-240: The fallback with --set-upstream duplicates the initial
push's -u flag; change the initial push call that builds args with '-u' so it
performs a plain push (remove '-u') and leave the fallback logic
(setUpstreamArgs + execGitCommand) intact so it only runs when upstream is truly
missing; locate the initial push invocation that calls execGitCommand (the
earlier push path that currently adds '-u') and remove the '-u' flag there, and
keep the fallback block using setUpstreamArgs/execGitCommand as-is (ensure
returned messages still reference branchName/targetRemote and that error returns
preserve pushError when appropriate).
- Around line 56-64: isDivergenceError currently treats any output containing
"rejected" as divergence which causes false positives; update the function to
only classify as divergence when stronger markers are present (e.g.,
"non-fast-forward", "failed to push some refs", "fetch first") or when
"rejected" appears together with other divergence indicators (e.g., "rejected" +
"non-fast-forward"/"fetch"); modify the isDivergenceError(errorOutput: string)
logic to require either specific phrases or a combination of markers (and
explicitly exclude common server-side rejection text like "pre-receive hook" or
"protected branch") so only true non-fast-forward/divergence cases trigger the
pull-then-retry path.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/push.ts`:
- Around line 34-44: When handling a failed push in the push route (the block
checking result.success and using isClientError), treat a result.error value of
'diverged' as HTTP 409 Conflict instead of 400: compute statusCode as 409 when
result.error === 'diverged' (keep 400 for other client-side validation errors
and 500 for server errors), update both error-response sites (the blocks around
result.success and the later block at ~67-72) to use this mapping, and then
remove the now-unused isClientError helper to avoid dead code.
In `@apps/server/src/routes/worktree/routes/set-tracking.ts`:
- Around line 15-75: Move all Git/business logic out of createSetTrackingHandler
and into a new service (e.g., services/set-tracking-service.ts) or add a
performSetTracking(worktreePath, remote, branch?) function to an existing
service; the handler should only validate req.body, call the service function
(which will call getCurrentBranch and execGitCommand, handle the detached HEAD
check, and throw meaningful errors), and translate the service result to the
HTTP response. Ensure the service returns a structured result matching the
current response shape (branch, remote, upstream, message) and that the route
catches service errors, logs with logError('Set tracking branch failed', error)
and returns getErrorMessage(error) in the JSON error payload.
In `@apps/server/src/routes/worktree/routes/sync.ts`:
- Around line 35-46: The error response JSON in the sync route currently omits
branch context even though performSync sets result.branch on failures; update
the error response in the failure block (the code that checks if
(!result.success) and builds the JSON with success, error, hasConflicts, etc.)
to include branch: result.branch so the client receives branch information on
pull/conflict/push-diverge failures.
In `@apps/server/src/services/push-service.ts`:
- Around line 112-116: The pushArgs currently always include '-u', which can
overwrite a branch's upstream; change the push preparation in push-service.ts so
'-u' is only added when needed — either when the caller explicitly requests
tracking (e.g., a new opt-in flag like setTracking) or when the branch has no
configured upstream; to implement, before pushing check the branch's upstream
status (via the existing helper or a git command) and only push with '-u', e.g.,
pushArgs = ['push', targetRemote, branchName] and conditionally
pushArgs.unshift('-u') or pushArgs.push('-u') when upstream is absent or
setTracking===true; keep the existing force handling on the pushArgs array.
In `@apps/server/src/services/sync-service.ts`:
- Around line 140-185: The retry branch that triggers when pushResult.diverged
intentionally calls performPush with autoResolve: true which internally performs
another pull-then-push, resulting in a pull → push (rejected) → pull → push
sequence; update the code around the if (pushResult.diverged) block (reference
pushResult.diverged and performPush) with a concise inline comment explaining
that the second pull is intentional to resolve races and must not be removed or
"optimized" away, and mention getErrorMessage/pullResult as context for the
surrounding error handling.
- Around line 68-208: performSync currently runs a multi-step pull+push flow
without emitting progress events; update performSync to accept an EventEmitter
(or create one via createEventEmitter passed from the route) and emit structured
events at key points: emit 'sync:started' at start (include worktreePath and
remote), 'sync:pull-complete' after performPull returns (include pullResult and
any conflict info), 'sync:push-start' before performPush, 'sync:push-retry' when
retrying with autoResolve, and terminal events 'sync:completed' or 'sync:failed'
with the final SyncResult; ensure performPull and performPush call sites in this
function remain unchanged but wire the emitter into retries and catch blocks so
errors use getErrorMessage and the emitter to stream failures (reference
performSync, performPull, performPush, getErrorMessage, and createEventEmitter).
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 734-783: Extract the duplicated per-remote UI into a reusable
component (e.g., RemoteSubMenuItems) and replace the two near-identical map
blocks with calls to that component; the component should accept props: remotes,
worktree, onAction (handler for clicking remote e.g., onPullWithRemote or
onPushWithRemote), actionDisabled (boolean or function per-remote using
isPulling/isPushing), actionIcon (Download/Upload), onSetTracking,
trackingRemote, and isGitOpsAvailable, and render the same
DropdownMenuItem/DropdownMenuSub structure (including the Set as Tracking Branch
item) so the only differences between the Pull and Push usages are the icon
prop, the onAction prop, and the actionDisabled prop.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts`:
- Around line 241-261: The return object from use-worktree-actions.ts is missing
the pending state for the setTrackingMutation; add an isSettingTracking property
(set to setTrackingMutation.isPending) alongside the other flags so consumers
can disable the "Set as Tracking Branch" UI and show a spinner while
handleSetTracking runs; update the exported return to include isSettingTracking
and ensure handleSetTracking remains unchanged.
In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx`:
- Around line 834-856: The three useCallback pass-throughs
(handleSyncWithRemoteSelection, handleSyncWithSpecificRemote,
handleSetTrackingForRemote) are unnecessary wrappers around handleSync and
handleSetTracking; remove these three functions and update call sites to pass
handleSync and handleSetTracking directly (or, if a different signature is
required, rename handleSyncWithRemoteSelection to clearly indicate it doesn't
perform selection). Ensure any props or handlers that previously expected the
wrapper signatures now accept the original handleSync(worktree[, remote]) and
handleSetTracking(worktree, remote) functions so there are no mismatched
parameters.
In `@apps/ui/src/hooks/mutations/use-worktree-mutations.ts`:
- Around line 208-227: The mutation currently throws a plain Error with only
result.error, discarding structured fields like hasConflicts, conflictFiles, and
conflictSource returned by the API; update the mutationFn in
use-worktree-mutations (the useSyncWorktree mutation) to throw the original
result object (or a custom error object that preserves result.hasConflicts,
result.conflictFiles, result.conflictSource and result.error) instead of new
Error(result.error), and adjust the onError handler signature in the same file
to accept the thrown object (inspect error.hasConflicts, error.conflictFiles,
error.conflictSource) so callers can differentiate conflictSource (e.g., 'pull'
vs 'stash') when handling sync failures.
In `@apps/ui/src/lib/electron.ts`:
- Around line 2303-2319: The mock implementation of setTracking uses a
misleading hardcoded fallback 'feature-branch' when branch is undefined; update
the function (setTracking) to use a clearer placeholder like 'current-branch' or
add a comment indicating this is a dynamic stand-in for the real server-resolved
branch, and ensure the returned result fields (branch, upstream, message)
reflect that placeholder so developers testing the "no branch specified" path
aren't confused by a concrete feature name.
In `@apps/ui/src/types/electron.d.ts`:
- Around line 1001-1020: Add a top-level optional diverged?: boolean to the
sync() return shape (the Promise result object) to mirror push() and allow
callers to detect a push-divergence race without parsing error strings; update
the sync declaration (function name: sync) to include diverged?: boolean
alongside the existing success/error/hasConflicts fields, or alternatively add a
JSDoc note on sync explaining why divergence is impossible if you intend to keep
the current type.
…g, extract duplicate rendering - Tighten isDivergenceError() to require specific marker combinations instead of matching 'rejected' alone (avoids false positives on pre-receive hooks) - Remove redundant -u flag from initial push; only use --set-upstream in fallback path when "no upstream" error is detected - Add targetBranch as final positional arg to git branch --set-upstream-to so it configures the correct local branch explicitly - Extract RemoteActionMenuItem component to deduplicate pull/push remote submenu rendering logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (2)
173-220: Redundantkeyprops onRemoteActionMenuItem's returned root elements
keyprops placed on a component's returned root elements (lines 175, 209) have no effect on React's list reconciliation — React readskeyonly from the element at the call site. The keys are already correctly applied at the call sites (key={remote.name}on lines 807 and 908), so these internal ones are misleading dead code.🧹 Remove the redundant internal keys
if (onSetTracking) { return ( - <DropdownMenuSub key={remote.name}> + <DropdownMenuSub>return ( <DropdownMenuItem - key={remote.name} onClick={onAction}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 173 - 220, Remove the redundant internal key props from the returned root elements of the RemoteActionMenuItem component: delete key={remote.name} on the DropdownMenuSub wrapper (the branch that returns the submenu) and the key={remote.name} on the standalone DropdownMenuItem return path, since keys are provided by the parent call sites and these internal keys are ignored by React's reconciliation.
1002-1015: Sync per-remote items don't use the newly extractedRemoteActionMenuItemThe sync submenu items inline the exact same DOM structure as the plain (no-
onSetTracking) variant ofRemoteActionMenuItem— icon, remote name, and truncated URL. SinceRemoteActionMenuItemwas extracted to deduplicate this pattern, not using it here leaves an inconsistency and diverges from the stated refactor goal.♻️ Use RemoteActionMenuItem for sync per-remote items
- {remotes.map((remote) => ( - <DropdownMenuItem - key={`sync-${remote.name}`} - onClick={() => isGitOpsAvailable && onSyncWithRemote(worktree, remote.name)} - disabled={isSyncing || !isGitOpsAvailable} - className="text-xs" - > - <RefreshCw className="w-3.5 h-3.5 mr-2" /> - {remote.name} - <span className="ml-auto text-[10px] text-muted-foreground max-w-[100px] truncate"> - {remote.url} - </span> - </DropdownMenuItem> - ))} + {remotes.map((remote) => ( + <RemoteActionMenuItem + key={remote.name} + remote={remote} + icon={RefreshCw} + trackingRemote={trackingRemote} + isDisabled={isSyncing} + isGitOpsAvailable={isGitOpsAvailable} + onAction={() => isGitOpsAvailable && onSyncWithRemote(worktree, remote.name)} + /> + ))}This also gains the tracking indicator for free when
trackingRemotematches a remote — consistent with how pull/push per-remote items behave.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 1002 - 1015, Replace the inline per-remote DropdownMenuItem JSX inside remotes.map with the extracted RemoteActionMenuItem to avoid duplication: for each remote, render RemoteActionMenuItem (keeping the same key) and pass the RefreshCw icon, the remote object/name, the onClick handler () => isGitOpsAvailable && onSyncWithRemote(worktree, remote.name), and the disabled prop (isSyncing || !isGitOpsAvailable); this preserves behavior and also lets RemoteActionMenuItem show the tracking indicator when trackingRemote matches the remote.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 173-220: Remove the redundant internal key props from the returned
root elements of the RemoteActionMenuItem component: delete key={remote.name} on
the DropdownMenuSub wrapper (the branch that returns the submenu) and the
key={remote.name} on the standalone DropdownMenuItem return path, since keys are
provided by the parent call sites and these internal keys are ignored by React's
reconciliation.
- Around line 1002-1015: Replace the inline per-remote DropdownMenuItem JSX
inside remotes.map with the extracted RemoteActionMenuItem to avoid duplication:
for each remote, render RemoteActionMenuItem (keeping the same key) and pass the
RefreshCw icon, the remote object/name, the onClick handler () =>
isGitOpsAvailable && onSyncWithRemote(worktree, remote.name), and the disabled
prop (isSyncing || !isGitOpsAvailable); this preserves behavior and also lets
RemoteActionMenuItem show the tracking indicator when trackingRemote matches the
remote.
1 participant
Summary
child_process.execwith safeexecGitCommandarray args in a newpush-service.ts, matching the existingpull-service.tspattern. Detects diverged branches and auto-resolves via pull-then-retry whenautoResolveis enabled.sync-service.ts+POST /sync), with automatic stash management and divergence retry. Appears as a new menu item between Push and Merge & Rebase with split-button support for multiple remotes.POST /set-trackingendpoint. Each remote in the Pull/Push submenus now has a nested sub-menu with "Set as Tracking Branch" — no duplicate remote lists.Changes
Server (4 new files, 2 modified)
apps/server/src/services/push-service.ts— New service withperformPush(), divergence detection, auto-resolveapps/server/src/services/sync-service.ts— New service composingperformPull()+performPush()apps/server/src/routes/worktree/routes/set-tracking.ts— NewPOST /set-trackingendpointapps/server/src/routes/worktree/routes/sync.ts— NewPOST /syncendpointapps/server/src/routes/worktree/routes/push.ts— Refactored to delegate topush-serviceapps/server/src/routes/worktree/index.ts— Register new routesFrontend (10 modified)
http-api-client.ts— Addedsync(),setTracking(), updatedpush()withautoResolveelectron.d.ts— Type signatures for new API methodselectron.ts— Mock implementationsuse-worktree-mutations.ts—useSyncWorktree(),useSetTracking()mutationsmutations/index.ts— Barrel exportsuse-worktree-actions.ts—handleSync,handleSetTracking,isSyncingworktree-actions-dropdown.tsx— Sync menu item + per-remote "Set as Tracking Branch" sub-menusworktree-tab.tsx,worktree-dropdown.tsx,worktree-panel.tsx— Prop wiringTest plan
npm run build:packages && npm run build:servernpm run lint(0 errors)🤖 Generated with Claude Code
Summary by CodeRabbit